Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: serialize buffer size check #47

Merged
merged 1 commit into from
Jun 17, 2024
Merged

fix: serialize buffer size check #47

merged 1 commit into from
Jun 17, 2024

Conversation

MichaHoffmann
Copy link
Collaborator

During the check that we dont overflow the serialization buffer we erroneously checked with size(uint32_t) == 1 which could cause us to crash with some input strings.

might fix #46

@MichaHoffmann MichaHoffmann force-pushed the mhoffmann/fix-size-accounting-check-in-serialize branch from 2bdfd70 to caf14e2 Compare June 8, 2024 16:08
@MichaHoffmann MichaHoffmann requested a review from amaanq June 8, 2024 16:08
@MichaHoffmann MichaHoffmann force-pushed the mhoffmann/fix-size-accounting-check-in-serialize branch from caf14e2 to bb5417a Compare June 9, 2024 15:48
@MichaHoffmann
fix: serialize buffer size check
8078460 
During the check that we dont overflow the serialization buffer we
erroneously checked with size(uint32_t) == 1 which could cause us to
crash with some input strings.

Signed-off-by: Michael Hoffmann <[email protected]>
@MichaHoffmann MichaHoffmann force-pushed the mhoffmann/fix-size-accounting-check-in-serialize branch from bb5417a to 8078460 Compare June 9, 2024 15:49
@amaanq amaanq merged commit e2d416a into main Jun 17, 2024
12 checks passed
@amaanq amaanq deleted the mhoffmann/fix-size-accounting-check-in-serialize branch June 17, 2024 04:54
@MichaHoffmann MichaHoffmann mentioned this pull request Jun 17, 2024
Closed
@clason
Copy link
Contributor

clason commented Jun 17, 2024 •
edited
Loading

@amaanq This change doesn't compile with tree-sitter build (nor with nvim-treesitter flags).

@MichaHoffmann
Copy link
Collaborator Author

@amaanq This change doesn't compile with tree-sitter build (nor with nvim-treesitter flags).

Oh sorry! We should add a CI pass for that. What are the flags and the compiler again? I'll see ti fixing it after work today.

@clason
Copy link
Contributor

clason commented Jun 17, 2024

Just tree-sitter build will do the trick. (You might wish to rewrite the scanner to use the new headers you get with tree-sitter generate -- @amaanq should be able to do this quickly, he has practice ;))

@clason
Copy link
Contributor

clason commented Jun 17, 2024

You could also use the upstream workflow while you're at it: https://github.com/tree-sitter-grammars/template/tree/master/.github/workflows

@MichaHoffmann
Copy link
Collaborator Author

Weird, I did run the tests and all multiple times using the tree sitter cli, I wonder why build should break if the compilation for tests worked.

@clason
Copy link
Contributor

clason commented Jun 17, 2024

Which CLI version?

@MichaHoffmann
Copy link
Collaborator Author 

 fedora  ~  git  tree-sitter-hcl   mhoffmann/fix-size-accounting-check-in-serialize   
$ tree-sitter build
 fedora  ~  git  tree-sitter-hcl   mhoffmann/fix-size-accounting-check-in-serialize   
$ tree-sitter
tree-sitter 0.22.5
...

@clason
Copy link
Contributor

clason commented Jun 17, 2024 •
edited
Loading

Could be compiler version-specific, of course. (Latest CLI -- and the one we test with -- is 0.22.6.)

And you're compiling from the dialects/terraform directory? (Multi-grammar repos are cursed.)

clason
clason reviewed Jun 17, 2024
View reviewed changes
src/scanner.c
TEMPLATE_INTERPOLATION,
TEMPLATE_DIRECTIVE,
QUOTED_TEMPLATE,
HEREDOC_TEMPLATE,
};
} ContextType;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You're missing these changes in the terraform scanner.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈 that explains it; sorry and thank you! Is there a way to handle the terraform scanner more elegantly? I put it there because at the time nvim-treesitter didnt allow having queries for multiple filetypes with one parser.

Copy link
Contributor

@clason clason Jun 17, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If it's really the same parser, you can register it for other filetypes with vim.treesitter.language.register(). But if the queries differ, this is pretty much the only way without manual query loading.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Queries differ somewhat ( there used to be only one, but then there were some complaints that some strings got highlighted as terraform keywords in other hcl based languages like for packer or whatnot, so i split it into generic hcl and then terraform iirc )

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Then I think this is the best for now (until upstream tree-sitter adds dedicated "dialect" support).

@clason
Copy link
Contributor

clason commented Jun 17, 2024 •
edited
Loading

I just pushed a fixup with the missing changes. @amaanq You probably still want to rewrite these scanners.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@clason clason clason left review comments

@amaanq amaanq amaanq approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Assertion Failed on Terraform Files
3 participants
@MichaHoffmann @clason @amaanq